Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update default python exporters to use OTLP #1328

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Dec 14, 2022

Closes #1168.

Updates the default exporters for python to use otlp and OTEL_EXPORTER_OTLP_[TRACES|METRICS]_PROTOCOL=http/protobuf

Depends on #1321

@TylerHelmuth TylerHelmuth requested a review from a team December 14, 2022 19:22
@mat-rumian
Copy link
Contributor

mat-rumian commented Dec 15, 2022

@TylerHelmuth shouldn't we use OTEL_METRICS_EXPORTER as otlp and then set OTEL_EXPORTER_OTLP_METRICS_PROTOCOL as http/protobuf for latest release to avoid warning logs which can be confusing for end users?
I think the same should be done for traces. It will make env vars configuration to be conformed with specification. What you think? :)

@TylerHelmuth
Copy link
Member Author

@mat-rumian sure I can make that change.

@TylerHelmuth TylerHelmuth changed the title Update default python metrics exporter Update default python exporters to use OTLP Dec 15, 2022
@TylerHelmuth TylerHelmuth force-pushed the update-python-metrics-exporter branch from 0e0534e to b5a2fd0 Compare December 15, 2022 17:56
@pavolloffay
Copy link
Member

Shall we as well change this comment in the requirements

# We don't use the distro[otlp] option which automatically includes exporters since gRPC is not appropriate for
?

So do I get it right that the exporter will use proto over HTTP and not gRPC?

@mat-rumian
Copy link
Contributor

Shall we as well change this comment in the requirements

# We don't use the distro[otlp] option which automatically includes exporters since gRPC is not appropriate for

?
So do I get it right that the exporter will use proto over HTTP and not gRPC?

We didn't use gRPC from the beginning. PR just changing the way exporter is configured. Previous configuration was setting up exporter to use proto and HTTP in single environment variable OTEL_TRACES_EXPORTER (like otlp_proto_http).
OT Python community developed support for configuration described in the OT Specification - so OTEL_x_EXPORTER which defines the exporter (e.g. zipkin, jaeger, otlp) and OTEL_EXPORTER_OTLP_x_PROTOCOL which defines protocol (e.g. grpc, http/protobuf, http/json).

@TylerHelmuth TylerHelmuth force-pushed the update-python-metrics-exporter branch 3 times, most recently from 615bdc3 to e8c7252 Compare December 20, 2022 19:53
@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Dec 20, 2022

@frzifus @pavolloffay @mat-rumian the e2e tests highlighted an interesting scenario that my code wasn't handling. Originally I was only setting the protocol when the exporter also needed set to otlp, but this doesn't work when the user has already set the the exporter to otlp themselves.

I've updated the code to be less intelligent and always set the otlp protocol env var if it is not set. This env var is unncessary when the exporter is set to something other than otlp, but it also shouldn't hurt anything. We could go the extra step and only set the protocol if it is not set AND the exporter is set to otlp, but that feels a little flimsy. I think always setting the protocol if it is not set will be more flexible as the spec/python auto-instrumentation finalize.

Please re-review.

@TylerHelmuth TylerHelmuth requested review from pavolloffay, mat-rumian and frzifus and removed request for pavolloffay, mat-rumian and frzifus December 20, 2022 19:57
@TylerHelmuth TylerHelmuth requested review from pavolloffay and frzifus and removed request for mat-rumian and pavolloffay December 20, 2022 19:57
@TylerHelmuth TylerHelmuth force-pushed the update-python-metrics-exporter branch from e8c7252 to f7a9077 Compare December 20, 2022 20:10
@pavolloffay pavolloffay merged commit fde6b95 into open-telemetry:main Jan 4, 2023
@TylerHelmuth TylerHelmuth deleted the update-python-metrics-exporter branch January 5, 2023 15:28
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Update default python metrics exporter

* Update sdk test

* Update python to use `otlp` and OTEL_EXPORTER_OTLP_*_PROTOCOL

* add changelog

* Update e2e tests

* Update how protocol is set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use python OTLP/proto metrics exporter once available
4 participants